fix: raise ValueError for unsorted ExplicitBucketHistogramAggregation boundaries#5340
Conversation
tests pass on local |
| instrument_aggregation_temporality | ||
| ) | ||
| self._start_time_unix_nano = start_time_unix_nano | ||
| for i in range(1, len(boundaries)): |
There was a problem hiding this comment.
Java also checks that first and last elements are not -inf and +inf, I guess it won't hurt
There was a problem hiding this comment.
They do it in a bit more efficient way, after they checked the boundaries are sorted they only check the first and last element.
|
good call added finite checks for NaN, +inf, and -inf, matching Java's full validation. tests added for all three cases |
| instrument_aggregation_temporality | ||
| ) | ||
| self._start_time_unix_nano = start_time_unix_nano | ||
| for i in range(1, len(boundaries)): |
There was a problem hiding this comment.
They do it in a bit more efficient way, after they checked the boundaries are sorted they only check the first and last element.
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
|
good catch, updated to match that pattern, also improved the error messages to show the actual violating values, and the validation check is shifted to the top so there is no half object intialization |
Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
|
Pushed the update for simplification, lmk if anything else comes up |
1 similar comment
|
Pushed the update for simplification, lmk if anything else comes up |
You might need to fix up the imports, otherwise LGTM! |
3 participants
Fixes #5338
What's wrong
ExplicitBucketHistogramAggregationaccepts unsorted boundary lists without any validation.bisect_left(used inaggregate()) requires sorted input with unsorted boundaries, measurements are silently placed in the wrong buckets with no error or warning.What this does
Adds a validation loop in
_ExplicitBucketHistogramAggregation.__init__that raisesValueErrorif boundaries are not strictly increasing. This matches the behavior of the Java SDK (IllegalArgumentException) and the Go SDK (non-monotonic boundarieserror).Tests
Added
test_unsorted_boundaries_raiseandtest_duplicate_boundaries_raisetoTestExplicitBucketHistogramAggregation.